-
Notifications
You must be signed in to change notification settings - Fork 465
fix: address Certora audit findings for Slashing UX Improvements #1693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**Motivation:** Claude Code is an AI-powered CLI tool that can assist with development tasks. Adding project-specific skills and instructions helps Claude understand EigenLayer conventions and automate common workflows like writing tests, extracting audit findings, and formatting interfaces. **Modifications:** - Add 4 Claude Code skills in `.claude/skills/`: - `audit-extractor`: Extract findings from PDF audit reports to markdown checklists - `integration-test-writer`: Write Solidity integration tests following project conventions - `interface-lint`: Format and lint Solidity interfaces with proper documentation - `unit-test-writer`: Write Solidity unit tests with per-function test contracts - Add `CLAUDE.md` with project-specific instructions, build commands, and conventions - Add slashing UX improvements audit report PDF and extracted findings markdown **Result:** Developers using Claude Code will have access to specialized skills that follow EigenLayer's testing and documentation conventions, improving productivity and code consistency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…1687) **Motivation:** Address L-01 finding from the Certora audit: "Instant slasher setting leaves stale slasher field in storage". When `_updateSlasher()` is called with `instantEffectBlock=true`, only `pendingSlasher` and `effectBlock` are updated, leaving the `slasher` field stale (address(0) for new operator sets). While `getSlasher()` returns correct values, raw storage is inconsistent. **Modifications:** - Updated `_updateSlasher()` in `AllocationManager.sol` to set `params.slasher = slasher` when `instantEffectBlock=true` - Updated NatSpec for `SlasherParams` struct in `IAllocationManager.sol` to document storage behavior - Added `getSlasherParams()` helper to `AllocationManagerHarness.sol` for testing raw storage - Added unit tests for instant slasher storage consistency - Updated documentation in `docs/core/AllocationManager.md` **Result:** - Storage is now consistent when creating operator sets or migrating slashers - Both `slasher` and `pendingSlasher` fields are set immediately when `instantEffectBlock=true` - Raw storage reads will return expected values matching `getSlasher()` return value 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…operators (#1688) **Motivation:** Addresses audit finding **L-01: State inconsistency for new operator allocation delay**. When a new operator registers through DelegationManager, their allocation delay is set via `_setAllocationDelay(..., newlyRegistered=true)`. Previously, only `pendingDelay` and `effectBlock` were updated, leaving `delay` and `isSet` at default values until applied. This created temporary storage inconsistency. **Modifications:** - `AllocationManager._setAllocationDelay()`: When `newlyRegistered=true`, now also sets `info.delay = delay` and `info.isSet = true` for immediate storage consistency - `IAllocationManager`: Updated NatSpec for `AllocationDelayInfo` struct to document the immediate update behavior - `AllocationManagerHarness`: Added `getAllocationDelayInfoRaw()` function for testing raw storage values - Unit tests: Added `test_setAllocationDelay_newlyRegistered_setsDelayAndIsSetImmediately` and `test_setAllocationDelay_existingOperator_delayNotSetImmediately` - Documentation: Updated `setAllocationDelay` Effects section in `docs/core/AllocationManager.md` **Result:** Raw storage in `_allocationDelayInfo[operator]` now correctly reflects the effective allocation delay immediately for newly registered operators. The `getAllocationDelay()` view function behavior remains unchanged (it already returned correct values by applying pending values in-memory). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…1689) **Motivation:** Address informational findings from the Certora audit of EigenLayer Slashing UX Improvements (PR-1645/PR-544). These are quality-of-life and documentation improvements that enhance code clarity and prevent potential edge case issues. **Modifications:** 1. **Fix I-01: Re-proposing same pending slasher is now a no-op** - Added check in `_updateSlasher()` to skip processing if the proposed slasher is already pending and hasn't taken effect - Prevents accidentally restarting the delay countdown when re-proposing the same slasher 2. **Fix I-02: Add NatSpec documentation for getSlasher/getPendingSlasher** - Updated interface and implementation NatSpec to document that these functions return `address(0)`/`0` for non-existent operator sets - Helps callers understand expected behavior without validation 3. **Fix I-03: Add separate SLASHER_CONFIGURATION_DELAY constant** - Added new `SLASHER_CONFIGURATION_DELAY` immutable in `AllocationManagerStorage` - Currently set to same value as `ALLOCATION_CONFIGURATION_DELAY` but can be changed independently in future upgrades - Updated `_updateSlasher()` to use the new constant - Added interface getter 4. **Fix I-05: Add gas warning documentation for migrateSlashers** - Added NatSpec warning about O(appointees) gas cost per operator set - `PermissionController.getAppointees()` enumerates full appointee set which can be expensive - Documented that large appointee sets may cause block gas limit issues **Result:** - `updateSlasher` is idempotent when re-proposing the same slasher - Clearer documentation on view function return values - Slasher delay can be configured independently in future upgrades - Users are warned about potential gas issues with `migrateSlashers` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
**Motivation:** Address informational findings from the Certora audit for Protocol Registry (PR-1655). These improvements enhance validation, prevent potential issues with orphaned configurations, and improve documentation clarity. **Modifications:** 1. **Fix I-01: ship() lacks validation** - Added array length validation: `addresses.length == configs.length == names.length` - Added zero address validation: revert if any address is `address(0)` - Added new errors: `ArrayLengthMismatch()` and `InputAddressZero()` 2. **Fix I-02: Orphaned configs on name overwrite** - When re-shipping a name with a new address, the old address's `DeploymentConfig` is now deleted - Added `DeploymentConfigDeleted(address)` event to signal cleanup - Prevents orphaned configs that could cause confusion 3. **Fix I-03: configure() for unshipped addresses** - Update `configure` function to pass in a name - Added validation that the address must be a shipped deployment before allowing configuration 5. **Fix I-04: Fix misleading NatSpec** - Updated `ship()` NatSpec to document that names can be re-shipped with new addresses - Clarified that old address configs are automatically deleted when this happens - Updated `configure()` NatSpec to indicate address must be previously shipped 6. **Fix I-05: Document pauseAll blocking** - Added warning documentation that `pauseAll()` will revert if ANY pausable deployment fails - A single misconfigured deployment can block the entire protocol pause - Important for operational awareness during emergency scenarios **Result:** - `ship()` validates inputs before processing - No more orphaned configs when re-shipping names - `configure()` only works for shipped addresses - Clearer documentation on expected behavior and edge cases - Operators understand risks of `pauseAll()` failure scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
e4af766 to
d300f6d
Compare
**Motivation:** The `ship()` function in ProtocolRegistry currently allows empty strings to be passed as contract names. Empty names are semantically meaningless and could lead to confusing registry state or lookup failures. **Modifications:** - Added `InputNameEmpty()` error to `IProtocolRegistryErrors` interface - Added validation in `ship()` to reject empty contract names with `require(bytes(names[i]).length > 0, InputNameEmpty())` - Added unit test `test_ship_revert_emptyName()` to verify the validation - Updated documentation to reflect the non-empty name requirement - Regenerated Go bindings **Result:** Calling `ship()` with an empty string in the `names` array will now revert with `InputNameEmpty()`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
0xClandestine
approved these changes
Jan 13, 2026
Member
0xClandestine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Certora Run Started (Verified Rules)
Certora Run Summary
|
Certora Run Started (Verified Rules)
Certora Run Summary
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Address findings from the Certora audit of EigenLayer Slashing UX Improvements. This branch consolidates fixes for low-severity findings (L-01) and informational findings (I-01 through I-05) across AllocationManager and ProtocolRegistry contracts.
Modifications:
AllocationManager Fixes
L-01: State inconsistency fixes
slasherfield immediately wheninstantEffectBlock=trueto prevent stale storage (fix: update slasher field immediately when instantEffectBlock=true #1687)delayandisSetfields immediately for newly registered operators to ensure storage consistency (fix: update allocation delay fields immediately for newly registered operators #1688)I-01: Re-proposing same pending slasher is now a no-op (#1689)
_updateSlasher()to skip processing if the proposed slasher is already pendingI-02: Add NatSpec documentation for getSlasher/getPendingSlasher (#1689)
address(0)/0for non-existent operator setsI-03: Add separate SLASHER_CONFIGURATION_DELAY constant (#1689)
ALLOCATION_CONFIGURATION_DELAYI-05: Add gas warning documentation for migrateSlashers (#1689)
ProtocolRegistry Fixes
I-01: ship() lacks validation (#1690)
ArrayLengthMismatch()andInputAddressZero()errorsI-02: Orphaned configs on name overwrite (#1690)
DeploymentConfigwhen re-shipping a name with a new addressDeploymentConfigDeleted(address)eventI-03: configure() for unshipped addresses (#1690)
configureto require a name parameterI-04: Fix misleading NatSpec (#1690)
ship()behavior when re-shipping namesconfigure()NatSpec for address requirementsI-05: Document pauseAll blocking (#1690)
pauseAll()reverts if ANY pausable deployment failsOther Changes
v1.9.0upgrade script fixes #1677)Result: